[박성우] 야구 게임 미션 by seong-wooo · Pull Request #1 · SoftwareMaestro-Backend-Study/java-baseball

seong-wooo · 2023-03-25T11:38:04Z

안녕하세요.

자바 애플리케이션을 오랜만에 만들어서 재밌었네요.

리뷰 해주실 때 상당히 비판적으로 코드를 봐주셨으면 좋겠습니다
왜 이따구로 코드를 짠거야... 라는 느낌으로요!

README는 작성하지 않았습니다.. 대신 테스트를 보기 쉽게 작성했어요!
그리고 뷰쪽 로직이 좀 더럽다는 느낌이 있습니다.
정리하기 어렵네요 . . ㅠ

남이 짠 코드, 리뷰 내용은 정답이 아닙니다.
각자의 의견을 자유롭게 작성해주세요~

jeongyuneo

전체적으로 이해가 잘됐고, GameRsult를 enum으로 처리하신 부분이 좋았습니다!
애플리케이션은 눈에 잘 안들어오는 것 같아요.
특히 테스트 코드 작성을 정말 잘 해주신 것 같아요. 테스트 코드 작성하신 거 보고 많이 배울 수 있을 것 같습니다ㅎㅎ

jeongyuneo · 2023-03-25T15:24:43Z

src/main/java/baseball/domain/GameNumbers.java

+    private GameNumbers(List<GameNumber> numbers) {
+        validateLength(numbers);
+        validateDuplicate(numbers);
+        this.numbers = numbers;


일급 컬렉션 생성 시 참조를 끊어주면 의도하지 않은 값의 수정을 막을 수 있을 것 같아요!

매의 눈이시네요. 수정 했습니다~

jeongyuneo · 2023-03-25T16:00:51Z

src/main/java/baseball/service/GameService.java

+    private final GameNumbers answerNumbers;
+
+    public GameService() {
+        this.answerNumbers = GameNumbers.from(new RandomIntegerToGameNumberCreator());


게임 서비스가 생성될 때마다 RandomIntegerToGameNumberCreator를 계속 생성할 필요는 없을 것 같은데 해당 클래스는 유틸 클래스로 만든건 어떨까요?

이 부분은 전략 패턴을 사용한 부분이라서 객체의 상태로 존재해야합니다.
근데 정윤님 말대로 계속 생성할 필요는 없을 것 같아요.
필드 변수로 선언했습니다!

jeongyuneo · 2023-03-25T16:10:08Z

src/main/java/baseball/domain/gamenumbercreator/StringToGameNumberCreator.java

+        if (values.length() != maxLength) {
+            throw new IllegalArgumentException(String.format("[ERROR] %s는 %d 자리수가 아닙니다.", values, maxLength));
+        }


검증 로직을 분리하면 좋을 것 같아요!

메서드 분리 반영했습니다~

jeongyuneo · 2023-03-25T16:23:59Z

src/main/java/baseball/domain/GameNumbers.java

+    public static GameNumbers from(GameNumberCreator numberCreator) {
+        return new GameNumbers(numberCreator.create(MAX_LENGTH));
+    }


생성자를 노출하지 않고, 이런 방식으로 객체를 생성하는 이유가 있을까요?

저는 생성자는 생성의 역할만 해야된다고 생각해서 이렇게 구현했습니다.
생성자에서 객체의 변환을 해야한다면 정적 팩터리 메서드를 사용해서 변환을 하고 생성자로 넘기는 방식을 사용합니다.

jeongyuneo · 2023-03-25T16:27:12Z

src/main/java/baseball/Application.java

+        }
+
+        if (strike != 0) {
+            answer.append(result.getStrike());


Suggested change

answer.append(result.getStrike());

answer.append(strike);

실수 했네요 . 디게 꼼꼼히 보신듯 :)

jeongyuneo · 2023-03-25T16:41:30Z

src/main/java/baseball/Application.java

+            System.out.println("게임을 새로 시작하려면 1, 종료하려면 2를 입력하세요.");
+            if (Console.readLine().equals("2")) {
+                System.out.println("게임 종료");
+                break;
+            }


해당 입력에 대해선 검증을 안하고 있어서 2가 아닌 값만 입력하면 게임을 새로 시작할 수 있겠네요.

이 부분 정윤님 코드 보면서 깨달았네요.
수정했습니다!

ray-yhc

구조도 깔끔하고 배울점이 많은 코드인 것 같아요!

ray-yhc · 2023-03-26T01:27:48Z

src/test/java/baseball/domain/gamenumbercreator/StringToGameNumberCreatorTest.java

+    @Test
+    void 정상적인_수로_게임_넘버를_만들_수_있다() {
+        // given
+        final StringToGameNumberCreator creator = new StringToGameNumberCreator("123");
+
+        // when & then
+        Assertions.assertThatThrownBy(() -> creator.create(3))
+                .isInstanceOf(IllegalArgumentException.class);
+    }


정상 케이스인데, .isInstanceOf(IllegalArgumentException.class) 가 실행되고 있어요!

오 실수했군요 감사합니다~

ray-yhc · 2023-03-26T01:34:10Z

src/main/java/baseball/domain/GameNumber.java

GameNumber라는 클래스를 정의해서 숫자와 관련된 로직을 여기에 모아두셨군요. 이렇게 하는게 더 코드를 관리하는 데에 편할 것 같네요! 배워갑니다~

다만 궁금한 점은, 번호를 Integer로 관리할 때 보다 차지하는 메모리가 커서 비효율적이지는 않을까요?

Integer도 객체라서 별 차이 없지 않을까요..??

ray-yhc · 2023-03-27T00:57:14Z

src/main/java/baseball/Application.java

+            System.out.print("숫자를 입력해주세요 : ");
+            String playerInput = Console.readLine();
+
+            GameResult result = gameService.findResult(playerInput);
+
+            String answer = getResultComment(result);
+            System.out.println(answer);
+            if (result.equals(GameResult.THREE_STRIKE)) {
+                break;


게임 진행 과정이 읽기 쉽게 작성되어서 좋은 것 같아요. 메소드명과 변수명도 통일성이 있어 연관성이 한눈에 들어오네요.

ray-yhc · 2023-03-27T01:02:43Z

src/main/java/baseball/Application.java

콘솔 입출력과 게임 진행이 모두 App 클래스에 모여있네요.
Controller와 Service를 분리하듯이, 입출력과 게임 진행 로직을 분리하면 어떨까 하는 의견입니다!

Application 클래스에서 게임 진행 로직과 view 로직을 분리하더라도 진행 흐름에는 변함이 없을 것 같네요.
view 쪽 로직은 크게 신경쓰지 않도록 하겠습니다..ㅎ

ray-yhc · 2023-03-27T01:16:19Z

src/main/java/baseball/domain/GameResult.java

Application의 getResultComment가 여기에 위치하면 어떨까 싶습니다.
만약 게임 룰이 변경되거나, 출력문구('낫싱'이 다른 문구로 변경되는 등등) 에 수정이 필요할 경우 GameResult를 수정하는 게 더 효율적일 것 같아서요!

사실 이 부분은 view 로직이라 크게 신경쓰진않았는데요.
이 애플리케이션에서는 GameResult 안에 view 로직이 들어가는게 더 효율적이겠네요!

ray-yhc · 2023-03-27T01:17:43Z

src/main/java/baseball/domain/gamenumbercreator/GameNumberCreator.java

GameNumberCreator를 이용해서 랜덤과 사용자 입력을 동일한 방식으로 처리해주는 게 너무 깔끔한 것 같아요! 배워갑니다 👀

ray-yhc · 2023-03-27T01:19:46Z

src/main/java/baseball/domain/GameNumbers.java

+    }
+
+    private void validateDuplicate(List<GameNumber> numbers) {
+        if (numbers.size() != new HashSet<>(numbers).size()) {


HashSet을 사용하니 훨씬 깔끔하군요..! 참고하겠습니다!

ray-yhc · 2023-03-27T01:21:48Z

src/main/java/baseball/domain/GameNumbers.java

+    public GameResult calculateResult(GameNumbers other) {
+        int strike = (int) IntStream.range(0, other.numbers.size())
+                .filter(i -> other.numbers.get(i).equals(this.numbers.get(i)))
+                .count();
+
+        int ball = (int) other.numbers.stream()
+                .filter(this.numbers::contains)
+                .count() - strike;
+
+        return GameResult.find(strike, ball);
+    }


Stream을 잘 활용하시네요! 저도 더 공부해봐야겠습니다.

ray-yhc · 2023-03-27T01:29:13Z

src/test/java/baseball/domain/GameNumbersTest.java

+    public static Stream<Arguments> invalidGameNumbers() {
+        return Stream.of(
+                Arguments.of(List.of(new GameNumber(1), new GameNumber(2), new GameNumber(3), new GameNumber(4))),
+                Arguments.of(List.of(new GameNumber(1), new GameNumber(2)))
+        );
+    }
+
+    public static Stream<Arguments> numbers() {
+        final List<GameNumber> computerNumbers = List.of(new GameNumber(1), new GameNumber(2), new GameNumber(3));
+
+        return Stream.of(
+                Arguments.of(
+                        computerNumbers,
+                        List.of(new GameNumber(1), new GameNumber(2), new GameNumber(3)),
+                        GameResult.THREE_STRIKE),
+                Arguments.of(
+                        computerNumbers,
+                        List.of(new GameNumber(2), new GameNumber(1), new GameNumber(3)),
+                        GameResult.ONE_STRIKE_TWO_BALL),
+                Arguments.of(
+                        computerNumbers,
+                        List.of(new GameNumber(7), new GameNumber(8), new GameNumber(9)),
+                        GameResult.ZERO)
+        );
+
+    }


오 이렇게 ParameterizedTest를 만들 수 있군요!

seong-wooo added 10 commits March 24, 2023 15:24

feat: 게임 숫자 생성 및 테스트

ca9e7af

feat: 게임 수들 생성 및 테스트

bdd44d3

feat: 입력값을 게임 수로 변경하는 기능 구현 및 테스트

e4e71af

feat: Integer값을 게임 수로 변경하는 기능 구현 및 테스트

3d209ad

refactor: 패키지 분리

de776a1

test: GameNumbersTest 테스트 케이스 추가

b0b21a5

feat: GameResult 생성 및 테스트

79a4760

feat: 게임 결과 계산 기능 추가 및 테스트

7a4efb6

feat: 게임 숫자 생성 시 예외 사항 추가

8a59a93

feat: Application 구현

201effb

seong-wooo requested review from great-park, jeongyuneo and ray-yhc March 25, 2023 11:38

seong-wooo self-assigned this Mar 25, 2023

jeongyuneo reviewed Mar 25, 2023

View reviewed changes

seong-wooo added 5 commits March 26, 2023 02:07

refactor: 일급 컬렉션의 참조를 끊음

a059027

refactor: 메서드 분리

b719d6c

refactor: strike 변수 직접 사용하도록 변경

31a0914

refactor: 계속 생성될 필요가 없는 객체를 필드 변수로 선언

5573e23

refactor: 재시작 입력값이 1,2가 아니면 예외가 발생하도록 변경

33a288f

ray-yhc reviewed Mar 27, 2023

View reviewed changes

seong-wooo added 3 commits March 27, 2023 12:31

refactor: 게임 끝 여부를 GameResult 내부로 숨김

0cf7b4c

refactor: 게임 결과 메시지 생성 GameResult 내부로 이동

06b7e3c

refactor: 잘못된 메서드명 수정

2d7c265

seong-wooo removed the request for review from great-park March 27, 2023 10:01

Conversation

seong-wooo commented Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

안녕하세요.

Uh oh!

jeongyuneo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seong-wooo Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ray-yhc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seong-wooo commented Mar 25, 2023 •

edited

Loading

seong-wooo Mar 25, 2023 •

edited

Loading